Skip to content

Performance enhancement: index iterator ancestry once per snapshot - #9838

Closed
JPPhoto wants to merge 2 commits into
invoke-ai:mainfrom
JPPhoto:if-iterator-ancestry-optimization
Closed

JPPhoto wants to merge 2 commits into
invoke-ai:mainfrom
JPPhoto:if-iterator-ancestry-optimization

Conversation

@JPPhoto

@JPPhoto JPPhoto commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Reuse one analysis during recursive If inference and preparation. Add shared correctness and operation-count evidence.

Summary

Build the filtered iterator graph and ancestry index once per snapshot. Reuse it across recursive If inference and preparation. Build lazily when a fallback queries ancestry.

Related Issues / Discussions

Related: #9789, which has overlapping If admission work. This change preserves compatibility and does not modify that PR. No issue is claimed as fixed.

QA Instructions

  • Focused graph runtime, ancestry, and materializer tests: 26 passed; 2 deselected. The deselected import tests abort with free(): invalid pointer, also reproduced on unchanged main.
  • Ruff 0.11.2 check and format check: passed.
  • git diff --check: passed.
  • Paired operation counts against the unchanged baseline, using the same fixtures:
Fixture Baseline This Implementation
Chain, 130 nodes / 129 edges, 128 direct queries 128 graph copies; 16,640 nodes / 16,512 edges filtered; 8,384 ancestry-edge visits 1 copy; 130 nodes / 129 edges filtered; 130 topological-node visits and 258 topological/propagation edge visits
Recursive runtime, 16,512 fallback queries 16,512 copies; 2,146,560 nodes / 2,130,048 edges filtered; 732,032 ancestry-edge visits 1 copy; 130 nodes / 129 edges filtered; 130 topological-node visits and 258 topological/propagation edge visits
Wide graph, 130 nodes / 129 edges, 2 direct queries 2 copies; 260 nodes / 258 edges filtered; 4 ancestry-edge visits 1 copy; 130 nodes / 129 edges filtered; 130 topological-node visits and 258 topological/propagation edge visits

Results matched at fixture sizes 16, 32, 64, and 128. Counts cover graph analysis only, not all inference or scheduler work. The wide fixture shows the eager index costs more traversal work for sparse queries; these are operation counts, not wall-clock or production workload claims.

Review

Independent review found no findings or suggestions. No code changes followed the review.

Compatibility / Rollout

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Meaningful regression coverage added / updated where needed; obsolete tests/code removed
  • Persisted-state and API changes include required migrations / compatibility validation
  • Relevant performance/efficiency opportunities considered; material claims have evidence
  • Material review findings resolved and relevant checks rerun
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@github-actions github-actions Bot added python PRs that change python files services PRs that change app services python-tests PRs that change python tests labels Oct 13, 2026
@JPPhoto JPPhoto moved this to 7.0 Theme: Modular Design in Invoke - Community Roadmap Oct 13, 2026

@Pfannkuchensack Pfannkuchensack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this against base 3d76b457d6 / head a3c177102f. I found no blockers: the refactor keeps the existing behavior, and the context lifetime is safe.

Checks:

  • CI is green.
  • Ruff 0.11.2 check/format and git diff --check pass.
  • On the PR, tests/app/services/shared/, tests/test_graph_execution_state.py and tests/test_node_graph.py give 1728 passed, 0 failed.
  • The two import tests your description says abort with free(): invalid pointer passed here (Linux).

Correctness

  • has_iterator_ancestor is equivalent to the old any(For/Iterate in nx.ancestors(state._iterator_graph(g), node_id)):
    • It uses the same _iterator_graph filter: Collect in-edges and For Final edges are dropped.
    • It checks strict ancestors only, so the node itself is not counted.
    • A missing node raises the same NetworkXError.
  • The context is scoped to one prepare() call (or one top-level _get_if_iteration_paths call). It is built lazily and never stored or serialized, so it needs no invalidation. Admission and materialization within one prepare() see the same g.

Findings (all minor)

1. Scope vs. title. The context only covers the literal-condition If ancestry fallback. The real prepare() hot path still rebuilds the filtered iterator graph on every admission check:

  • _get_parent_iteration_mappings (graph_materializer.py:1322)
  • _get_if_condition_iteration_mappings
  • argument-less get_node_iterators() (1441-1445, 1466, 1610, 1821, 1069)

A depth-20 nested-literal-If profile with 200 padding nodes shows 5,643 _iterator_graph builds (~7.5s of ~10.2s). Only 39 of them go through the new index. _IteratorAncestry builds the filtered graph privately and exposes only a boolean, so those consumers can't share it. A follow-up that carries the filtered iterator graph per prepare() would probably need to reshape this class.

The README describes the scope accurately; the title ("index iterator ancestry once per snapshot") reads broader than the change.

2. End-to-end effect is small. The operation-count tables use direct-query fixtures. Full next()/complete() runs (min of 3, OMP_NUM_THREADS=1):

Graph Base PR
Nested literal Ifs, depth 20 (230 → 20 builds) 311 ms 292 ms (~6%)
Same, with 200 padding nodes 6762 ms 6648 ms (~1.7%)

The other representative graphs never hit the fallback, and their build counts are identical:

  • Iterate + Ifs
  • 250-node chain into Ifs
  • plain Iterate→Collect
  • wide Add/If fan-out

Across the three suites above, the fallback fired 839 times, all but 2 of them from this PR's own unit tests.

The admitted sparse-query cost is ~1.6× per single query (247 → 388 µs at 132 nodes, 2.07 → 3.48 ms at 1,002 nodes). It is paid at most once per prepare() and only when the fallback fires, so it's not a realistic regression. Still, the net win is modest; maybe phrase the PR as groundwork rather than a performance fix.

3. Tests are coupled to internals.

  • test_graph_if_runtime.py:177-205 and test_graph_materializer.py:73-122 count builds by monkeypatching the private GraphExecutionState._iterator_graph.
  • The materializer test passes an nx.subgraph_view that hides condition nodes, which production never does.
  • A fixture through next()/complete() would cover the same reuse with real output expectations, e.g. a dynamic inner If feeding a chain of literal outer Ifs, like test_nested_if_literal_condition_activates_selected_branch.
  • test_new_context_observes_supplied_graph_mutations adds little, since there is no retained cache it could catch.
  • The random-DAG equivalence test is good.

Nits

  • graph_if_runtime.py:16-24, 70-81: both source_graph and iterator_ancestry (bound to its own graph) are passed through. They could drift if a future caller passes mismatched ones. Every caller is consistent today.
  • README: "one compatibility prepare() call". _GenericForPlanner.prepare (graph_for_planner.py:29) also goes through _ExecutionNodeBuilder.prepare.

Heads-up

  • Conflict with #9843: this PR conflicts with #9843 in graph_if_runtime.py, because #9843 rewrites _get_effective_iteration_paths_for_edge iteratively. Whichever lands second needs to thread iterator_ancestry through the new loop.

  • Pre-existing stall, unrelated to this PR: the graph below runs nothing and stays is_complete() == False on both base and this PR. next() returns None immediately.

    IntegerCollection([1,2]) → Iterate → Add(b=10) → If(condition=True).true_input → Collect.item
    
    • With two parallel Ifs it completes.
    • Suspected cause: the collection source waits on the If's activation dependency, while the If waits for its Iterate ancestor.
    • Probably worth a separate issue (near #9789).

@JPPhoto
JPPhoto marked this pull request as draft October 13, 2026 18:59
@JPPhoto

JPPhoto commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator Author

I am abandoning this branch in favor of merging it into #9843 in order to avoid merge conflicts and additional review cycles.

@JPPhoto
JPPhoto force-pushed the if-iterator-ancestry-optimization branch from c51d47a to 0c78fe2 Compare October 13, 2026 19:21
@JPPhoto JPPhoto mentioned this pull request Oct 13, 2026
5 of 7 tasks
Reuse one analysis during recursive If inference and preparation.
Add shared correctness and operation-count evidence.
@JPPhoto
JPPhoto force-pushed the if-iterator-ancestry-optimization branch from 0c78fe2 to dd1e841 Compare October 14, 2026 11:11
@JPPhoto JPPhoto closed this Oct 14, 2026
@JPPhoto
JPPhoto deleted the if-iterator-ancestry-optimization branch October 14, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python PRs that change python files python-tests PRs that change python tests services PRs that change app services

Projects

Status: 7.0 Theme: Modular Design

Development

Successfully merging this pull request may close these issues.

3 participants